-
Notifications
You must be signed in to change notification settings - Fork 206
switch titiler.xarray to obstore+zarr and add default application #1253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| """test reader.""" | ||
| src_path = protocol + os.path.join(protocol, prefix, filename) | ||
| with Reader(src_path, variable="dataset") as src: | ||
| with Reader(src_path, variable="dataset", opener=fs_open_dataset) as src: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reader(src_path,, opener=fs_open_dataset, ...) is the same as titiler.xarray.io.FsReader
|
|
||
| # Fallback to Zarr | ||
| else: | ||
| store = zarr.storage.FsspecStore.from_url( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed support for zarr-python 2.0
| strategy: | ||
| matrix: | ||
| python-version: ['3.10', '3.11', '3.12', '3.13'] | ||
| python-version: ['3.11', '3.12', '3.13'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because titiler.xarray cannot support 3.10, it's just easier to also switch all package to >=3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding titiler.xarray.main! I ran uv run uvicorn titiler.xarray.main:app and it worked great out of the box, but I did have to set AWS_SKIP_SIGNATURE=True to get anonymous reads by default. I wonder how we might configure that type of setting at the application level.
| if "region" not in kwargs and infer_region: | ||
| # infer region or fallback to env variables | ||
| region_name_env = ( | ||
| os.environ.get("AWS_REGION", os.environ.get("AWS_DEFAULT_REGION")) | ||
| or None | ||
| ) | ||
| else: | ||
| store = fsspec.filesystem(protocol).get_mapper(src_path) | ||
| config["region"] = _find_bucket_region(parsed.netloc) or region_name_env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe throw an http exception if region can't be discovered
| fs = fsspec.filesystem(protocol, **kwargs) | ||
| ds = xarray.open_dataset(fs.open(src_path), **xr_open_args) | ||
|
|
||
| # Fallback to Zarr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to take zarr support out of this opener and have the application layer choose xarray_open_dataset or open_zarr? maybe it is nice to make this one reader that can open anything though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory you could but I think for compatibility issue it's better to keep everything separated.
maybe it is nice to make this one reader that can open anything though.
User could do it if they need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
That's a main issue with We're already have a |
It isn't pretty, but could we add Actually, if we add |
|
The we could add |
|
Ok I've changed some logic As mentioned before if user want to access |
| if not expr.groupdict().get("region"): | ||
| config["region"] = ( | ||
| _find_bucket_region(bucket) or region_name_env | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to support old style URL https://{bucket}s3.amazonaws.com/...
|
FYI this type of wrapper - virtual-zarr/obspec-utils#1 - provides a way to use xarray + obstore with libraries that expect file handles (e.g., netcdf4/h5netcdf). That would help if the goal is just to limit dependencies, but wouldn't help if the goal is to simplify code complexity by removing support for data formats other than zarr. |
ref #1247
closes #1084
This PR does:
open_zarropenerToDo